docs: correct three claims that do not match the code - #499
Merged
Merged
Conversation
Phase 32 truth-up. Each was verified against the code before being changed, and one of the three turned out to be less wrong than previously recorded. StreamingCleanConfig.window_size was documented as sizing "rolling statistics and the rolling trust score". It does neither. Its only functional use in the whole src tree is bounding the cross-batch duplicate window, and the rolling trust score is sized by a different field, rolling_trust_window. A user reading help(StreamingCleanConfig) would raise window_size expecting more history to feed trust scoring, and nothing would change. The prose docs in docs/streaming.md were already correct; the API docstring was the wrong one. docs/repair-plans.md described the drift fingerprint as "row count, column names+dtypes, content sample". That is accurate but incomplete in a way that matters: the sample is the first 512 rows, so a change after row 512 that preserves the row count, names and dtypes is not detected. Demonstrated: on a 1000-row frame the same edit raises PlanDriftError at row 10 and passes silently at row 900. The fingerprint is deliberately cheap and this is a design choice, not a bug -- but the head restriction belongs where the guarantee is described. A new test pins the boundary exactly at row 512 so the documented claim is backed by execution, and so a future change to the sampling strategy has to come past a failing test. The README's "Native Polars DataFrames" section showed fd.clean(pl_df) with defaults and called it native. docs/fallback-matrix.md opens by calling this "the single most important row": with default options every native engine delegates the whole pipeline to pandas. You get a Polars frame back, but not native Polars execution. That sentence now appears beside the example. Documentation and one new test only; no behaviour change.
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
FreshData benchmark report —
|
| fixture | n_rows | n_cols | p50 s | p95 s | peak MB | repair % | false-repair % | preserve % | trust | monotonic | export % |
|---|
Authored-code reduction (Metric 6)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Phase 32 documentation truth-up. Three documented claims were checked against the code; all three were wrong, though one was less wrong than I had previously recorded.
1.
StreamingCleanConfig.window_size— the docstring is factually wrongsrc/freshdata/streaming/_config.py:21-24said:Both halves are false:
rolling_trust_window(default 20) — whichdocs/streaming.md:100confirms: "mean of per-batch trust scores over the lastrolling_trust_windowbatches".window_sizehas exactly one functional use in the entiresrc/tree —_cleaner.py:391,while len(seen) > self.scfg.window_size, bounding the cross-batch dedup hash window. Established by exhaustive grep, excluding docstrings, comments, the dataclass default and the validation loop.A user reading
help(StreamingCleanConfig)would raisewindow_sizeexpecting more history to feed trust scoring, and nothing would change. The prose docs were already correct; the API docstring was the wrong one.2. Plan drift refusal is head-sampled — now stated, and pinned by a test
docs/repair-plans.mddescribed the fingerprint as "row count, column names+dtypes, content sample". That is accurate but incomplete in a way that matters: the sample is the first 512 rows, so a change after row 512 that preserves row count, column names and dtypes is not detected.Demonstrated on a 1000-row frame — the same edit, two positions:
PlanDriftErrorraisedThis is a deliberate design choice: the fingerprint is documented as "cheap", and it guards against applying a plan to the wrong data, not against the data having changed. But the head restriction belongs where the guarantee is described.
tests/test_plan_drift_sampling.py(5 tests) pins the boundary exactly at_SIGNATURE_SAMPLE_ROWS: an edit at index 511 changessample_hash, an edit at index 512 does not, and the row-count and column-name components still fire beyond the window. The documented claim is now backed by execution, and a future change to the sampling strategy has to come past a failing test.Correction to an earlier internal note: I had recorded that neither the docs nor the docstring disclosed the sampling. That was wrong — the docs did say "content sample" and the code is honest internally (
sample_hash, "a head sample"). What was missing was only that the sample is the head, and its size.3. The README called the default Polars path "native"
docs/fallback-matrix.mdopens by calling this "the single most important row":The README's "Native Polars DataFrames" section showed
fd.clean(pl_df)— the defaults, i.e. the configuration that delegates entirely to pandas — described as getting a Polars result "with zero pandas boilerplate". True of the type, not of the execution. That sentence now appears beside the example.Scope
Documentation and one new test. No behaviour change, so no compatibility impact. CHANGELOG entry added under
[Unreleased] / Documentation.Verification
ruff check .clean (the repo's CI lint)tests/test_plan_drift_sampling.py— 5 passedOpen question, deliberately not answered here
The README claims "Polars backend delivers 2–3× throughput vs pandas at 10M rows" and "DuckDB backend consumes 200 MB peak RAM at 1M rows vs 1,046 MB for pandas". The harnesses differ in configuration —
benchmarks/harness_metrics.py:240andbench_report.pyusestrategy="balanced"(the default, which delegates to pandas), while onlybench_native_semantic.pyuses the genuinely nativestrategy="conservative", fix_dtypes=False. I could not determine which harness produced the README figures, so I make no claim about them and have changed nothing there. Flagging it as worth confirming separately.